Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] add auto merge background task #298

Merged
merged 3 commits into from
Jan 4, 2024

Conversation

LindaSummer
Copy link
Contributor

Issue

#297

We need to add an auto merge background task with cron expression scheduler.

Proposed Changes

  • add cron task for auto merge with cron library.
  • add related unit tests

@LindaSummer
Copy link
Contributor Author

Hi @roseduan, please take a look. Thanks. 😊

I have one problem when writing unit test for auto merge.

I don't know how to check whether merge action has been operated without an internal flag.

I think I should add such a unit test for case coverage.

Best Regards

@roseduan
Copy link
Collaborator

Hi @roseduan, please take a look. Thanks. 😊

I have one problem when writing unit test for auto merge.

I don't know how to check whether merge action has been operated without an internal flag.

I think I should add such a unit test for case coverage.

Best Regards

Thanks for your PR.
What does 'internal flag' mean?

Merge operation will discard all invalid data, and rewrite all valid data into new data files.
So maybe you can check the valid keys, for example, you put 10 keys, and delete 5 keys, after merge, all keys num will be 5.

@LindaSummer
Copy link
Contributor Author

Hi @roseduan, please take a look. Thanks. 😊
I have one problem when writing unit test for auto merge.
I don't know how to check whether merge action has been operated without an internal flag.
I think I should add such a unit test for case coverage.
Best Regards

Thanks for your PR. What does 'internal flag' mean?

Merge operation will discard all invalid data, and rewrite all valid data into new data files. So maybe you can check the valid keys, for example, you put 10 keys, and delete 5 keys, after merge, all keys num will be 5.

Hi @roseduan , thanks very much for your warm suggestion.

I'm sorry for lack of explanation for 'internal flag'.

Previously I wanted to add a flag for merge actions and write unit test with it. But it makes no sense for production code.

I'll use keys' total count for unit test.

Happy New Year! 🎉

Best Regards

@LindaSummer
Copy link
Contributor Author

Hi @roseduan, please take a look. Thanks. 😊
I have one problem when writing unit test for auto merge.
I don't know how to check whether merge action has been operated without an internal flag.
I think I should add such a unit test for case coverage.
Best Regards

Thanks for your PR. What does 'internal flag' mean?

Merge operation will discard all invalid data, and rewrite all valid data into new data files. So maybe you can check the valid keys, for example, you put 10 keys, and delete 5 keys, after merge, all keys num will be 5.

Hi @roseduan , please take a look.

I have use datafile's key total count to make unit test.

It works now! Thanks very much for your suggestion.

@roseduan
Copy link
Collaborator

roseduan commented Jan 3, 2024

Thanks, the CI job timeout is 10 minutes, please fix the unit tests.

There are some other failures:
image

@LindaSummer
Copy link
Contributor Author

Thanks, the CI job timeout is 10 minutes, please fix the unit tests.

There are some other failures

Hi @roseduan , sorry for forgot adding cleanup code for my unit tests.

I have ran this commit in my own repo's GitHub Actions.

Please take a look.

Thanks very much,
Best Regards

@roseduan
Copy link
Collaborator

roseduan commented Jan 4, 2024

Thanks for your great work, looking forward to your next PR!

@roseduan roseduan merged commit f31d45e into rosedblabs:main Jan 4, 2024
2 checks passed
@LindaSummer
Copy link
Contributor Author

Thanks for your great work, looking forward to your next PR!

Thanks for your warm help.

I'm satisfied to make some contribution. 😊

Best Regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants